Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #24037 : Voice search should not be possible in PIP #24296

Closed
wants to merge 1 commit into from
Closed

For #24037 : Voice search should not be possible in PIP #24296

wants to merge 1 commit into from

Conversation

indurs
Copy link
Contributor

@indurs indurs commented Mar 16, 2022

File modified
AndroidManifest.xml

Added these following attributes to VoiceSearchActivity so that the activity doesn't go into the app's default task and it won't be included in the recents screen

android:taskAffinity=""
android:excludeFromRecents="true"

pip.mp4

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@indurs indurs requested review from a team as code owners March 16, 2022 11:02
@firefoxci-taskcluster
Copy link

No Taskcluster jobs started for this pull request 
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

Mugurell
Mugurell reviewed Mar 16, 2022
View reviewed changes
Copy link
Contributor

@Mugurell Mugurell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix! Thank you!

Just one remark about the commit message: it's great to have one giving more context or explaining the reasoning for the approach but in this case listing the updated files or the changes made is not really needed since all of this is available at a quick glance in the commit.
Can you please update the commit message to not include these?

The commit generally also has a title synthesizing the result after the patch so in this case everything could be (just as an example):

For ..: Do voice search in a different task

This resolves voice search breaking Picture-in-Picture while having no visible effect for the in-app voice search functionality.

I do appreciate these guidelines from Googe - https://github.com/google/eng-practices/blob/master/review/developer/cl-descriptions.md about how to structure commits.

@indurs
Copy link
Contributor Author

indurs commented Mar 17, 2022

@Mugurell , Thank you for the feedback !! I appreciate it. Will update the commit message

@indurs
Copy link
Contributor Author

indurs commented Mar 17, 2022

@Mugurell , I've updated the commit message.

For mozilla-mobile#24037: Do voice search in a separate task
70b61c4 
This allows voice search to open in a separate window instead of opening in the same Picture-In-Picture window,  thereby making it easier to interact with the voice search.
@indurs indurs force-pushed the issue24037fix-branch branch from f54fefb to 70b61c4 Compare March 17, 2022 05:56
@Mugurell Mugurell mentioned this pull request Mar 17, 2022
Merged
3 tasks
@Mugurell
Copy link
Contributor

Thank you @indurs!
Will land in #24310

@Mugurell Mugurell closed this Mar 17, 2022
@indurs
Copy link
Contributor Author

indurs commented Mar 17, 2022

Sure. Thank You !!

@indurs indurs deleted the issue24037fix-branch branch March 19, 2022 00:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Mugurell Mugurell Mugurell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@indurs @Mugurell